[ENG-8064] Add New Notifications Data Model (Refactor Notifications Phase 2) by Johnetordoff · Pull Request #11151 · CenterForOpenScience/osf.io

Johnetordoff · 2025-05-20T14:09:45Z

Purpose

This system is designed to formalize and consolidate OSF Notifications under one system in order to better facilitate collaboration between Product, Engineers and QA and end persistent problems with notification email related tech debt. This project is the result of an extensive audit of all OSF emails and combined email digests and seeks to move the NotificationSubscription model out of it's transitional state having been migrated from a document based model, into it's final data model which is more regular for a relation our current relational database (django's postgress db).

In order to do this I have taken @mfraezz design documents and implemented them with minor changes. This means the responsibility for sending notifications in osf.io is shared by three new models, which will have the old data migrated into them via a migration script and management command. The models are:

NotificationType
- Similar to RegistrationSchemas or Waffle flags these are in db instrances which are poulated from static config documents in this case yaml.
- Since these are synced on migration with notification.yaml a developer will be able to see at a glance if where a notification template is and what it's purpose is.
NotificationSubscription
- This model is somewhat analogous to the earlier EmailDigest model, this holds references to potentially many Notifcations models that can be compiled into a single digest this is sent at a periodic basis.
Notification
- Holds message information and context
- Unlike earlier implementations this will record each Notification creation and sending for metrics purposes.

Changes

Combines worker with beat in docker-compose
creates notifications.yaml to contain all notification types info it is populated via postmigrate hook
creates new Notification NotificationSubscription and NotificationType
adds migrations to rename legacy tables and a managment command to populate the new ones.
Deletes old send_mails method and replaces it with emit of NotificationType
adds tests and updates old mocking
updates views and permission classed
A slight change to handle_boa_error to pass the already decanted user object.
adds new data model for Notification, NotificationTypes and Subscriptions
creates a notifications.yaml for data dependency notificationtypes
add migrations
updates notifications to use NotificationTypes
updates Admin app to control email templates
updates metrics reporter to count notifications sent etc.
updates tests to all use capture_notifications mocking util
Removes queued_mail
Removes EmailDigest
Removes detect_duplicates for duplicate subscriptions
Removes unused management commands that sent notifications.

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-8064

…/fix/reviews_signals [ENG-8851][ENG-8852] Preprint Moderator Reject: No emails sent when moderator rejects preprint submission

…/fix/registration_fail_notification [ENG-8871] Registrations: No notifications sent when registration fails.

…otifications-send-when-account-merged [ENG-8869] Fix/make send email on account merge

Johnetordoff · 2025-09-15T13:34:41Z

    _check_resource_permissions(resource, auth, action)

    provider_name = waterbutler_data['provider']
-    waterbutler_settings = None


…/fix/file_event_notifications [ENG-8861] File Operations: Email content and subject not correct when file is renamed

…otification-send-when-primary-email-changed [ENG-8872] Fix/no notification send when primary email changed

…/fix/submission_notifications [ENG-8848] Preprint pending submission notifications: Content of email is not relevant when new preprint is submitted to OSFPreprints

Johnetordoff · 2025-09-16T15:24:31Z

+        )
+
+    def remove_user_from_subscription(self, user):
+        """


Change docstring

Johnetordoff · 2025-09-16T15:29:03Z

+                f"\nemail_context={email_context}"
+
+            )
+        if self.message_frequency == 'instantly':


Maybe use enum or not litteral string.

Johnetordoff · 2025-09-16T15:32:37Z

+        Node/etc: <guid>_<event>
+        """
+        # Safety checks
+        event = self.notification_type.name


File updates are the only event after comment removal.

cslzchen

Progress update: 152/389 (last file is osf/models/notification.py)

On blocker I see is we modify old migrations again, which I suggest make new migrations. This is something that delayed phase 1.

In addition to my comments:

I see a bunch of management commands removed? Some are unrelated with notification refactor. Have you double checked that they are no longer used? They may have usage outside admin app.
I see you have 3-weeks old comments for yourself but many of them never addressed. Can your provide an update on your own comments?

cslzchen · 2025-09-16T18:36:09Z

    Guid,
    FileVersionUserMetadata,
-    FileVersion
+    FileVersion, NotificationType


LOW: fix style

cslzchen · 2025-09-16T18:53:47Z

+                if payload.get('email'):
+                    notification_type = NotificationType.Type.USER_FILE_OPERATION_SUCCESS.instance
+                if payload.get('errors'):
+                    notification_type = NotificationType.Type.USER_FILE_OPERATION_FAILED.instance


I may not understand the details yet but should we check 'errors' first?

i.e. when we (if we can) have both 'emails' and 'errors', checking 'emails first ignores the 'errors'

In addition, should we raise exceptions if neither exists

cslzchen · 2025-09-16T19:02:27Z


    logger.info('Successfully uploaded query output to OSF.')
    logger.debug('Task ends <<<<<<<<')
-    await sync_to_async(send_mail)(


As discussed, I couldn't remember why but I am wondering why we have to change it. Does keeping it async break notification refactor?

cslzchen · 2025-09-16T19:07:55Z

        # create AdminProfile for this new user
        profile, created = AdminProfile.objects.get_or_create(user=osf_user)
-
-        for group in form.cleaned_data.get('group_perms'):


Is this "on demand" thing part of the new refactor design? Or this is an old code clean up unrelated to refactor?

cslzchen · 2025-09-16T19:09:16Z

        context = super().get_context_data(**kwargs)
        node = self.get_object()

-        detailed_duplicates = detect_duplicate_notifications(node_id=node.id)


I assume this is part of the refactor design too?

cslzchen · 2025-09-17T03:46:25Z

+        @cached_property
+        def instance(self):
+            obj, created = NotificationType.objects.get_or_create(name=self.value)
+            return obj


As discussed, we don't have any implementation to clear the cache unless. What is the case where we need to clear cache? What could go wrong if we don't? At least I'd like to see this discussed and documented for post-release work.

cslzchen · 2025-09-17T03:54:53Z

    reviews_comments_anonymous = models.BooleanField(null=True, blank=True)

-    DEFAULT_SUBSCRIPTIONS = ['new_pending_submissions']
+    DEFAULT_SUBSCRIPTIONS = [


What's the issue here, has this comment been figured out?

cslzchen · 2025-09-17T03:56:53Z

            # remove notification subscription
            for subscription in self.DEFAULT_SUBSCRIPTIONS:
-                self.remove_user_from_subscription(user, f'{self._id}_{subscription}')
+                self.remove_user_from_subscription(user, subscription)


cslzchen · 2025-09-17T04:03:07Z

                self.add_permission(contrib.user, permission, save=True)
        Contributor.objects.bulk_create(contribs)

-    def subscribe_contributors_to_node(self):


So how is this done after the refactor? Or maybe this is just dead code?

Never used even on develop

fix file renamed email fix email digest fix digest formating

opaduchak

Progress: 100%

I've mostly dedicated my attention to main code, loooking more shallow on tests and templates.

My only main concern is: Some test classes and signal handlers are entirely removed, even through they are not directly related to notifications refactor.
It's hard to track if or whether they have been moved in PR of this scale

opaduchak · 2025-09-17T11:06:42Z


    logger.info('Successfully uploaded query output to OSF.')
    logger.debug('Task ends <<<<<<<<')
-    await sync_to_async(send_mail)(


I don't think so, but on the other hand async_to_sync is quite expensive operation (takes nearly 1ms according to django docs)

My opinion is that we need to refactor this entire method to get rid of async, as the only time it is being used is inside of celery task, so we have none of the benefits of async, only the drawbacks

opaduchak · 2025-09-17T11:14:36Z

+        context['provider__id'] = provider._id
+        context['is_reviews_moderator_notification'] = True
+        context['referrer_fullname'] = user.fullname
+        context['user_fullname'] = user.fullname


Nit: this has already been set on L353

opaduchak · 2025-09-17T11:43:54Z

+        f_type, action = self.action.split('_')
+        if self.payload['metadata']['materialized'].endswith('/'):
+            f_type = 'folder'
+        return '{action} {f_type} "<b>{name}</b>".'.format(
+            action=markupsafe.escape(action),
+            f_type=markupsafe.escape(f_type),
+            name=markupsafe.escape(self.payload['metadata']['materialized'].lstrip('/'))
+        )


Should not this be inside of mako template?

Do you mean the markupsafe for the whole return string? It's probably doesn't need to escape twice, but that's the way this works, these events are compiled as a list of html form matted string within the template.

opaduchak · 2025-09-17T11:46:56Z

+@register(NodeLog.FILE_REMOVED)
+class FileRemoved(FileEvent):
+    pass
+
+
+@register(NodeLog.FOLDER_CREATED)
+class FolderCreated(FileEvent):
+    pass


I believe they are inherited from FileEvent

opaduchak · 2025-09-17T11:53:41Z

+    except (SGUnauthorizedError, SGForbiddenError) as exc:
+        body = getattr(exc, 'body', b'')
+        try:
+            body = body.decode('utf-8', 'ignore') if isinstance(body, (bytes, bytearray)) else str(body)
+        except Exception:
+            pass
+        logging.error('SendGrid auth error (%s): %s', exc.__class__.__name__, body)
+        raise
+
+    except SGHTTPError as exc:
+        body = getattr(exc, 'body', b'')
+        try:
+            body = body.decode('utf-8', 'ignore') if isinstance(body, (bytes, bytearray)) else str(body)
+        except Exception:
+            pass
+        logging.error('SendGrid HTTPError: %s | payload=%s', body, payload)
+        raise


this may be deduplicated

Be more specific somethings may appear duplicated but the exact wording points to the exact cause of error.

Something like this, just noticed a lot of repetition here

Suggested change

except (SGUnauthorizedError, SGForbiddenError) as exc:

body = getattr(exc, 'body', b'')

try:

body = body.decode('utf-8', 'ignore') if isinstance(body, (bytes, bytearray)) else str(body)

except Exception:

pass

logging.error('SendGrid auth error (%s): %s', exc.__class__.__name__, body)

raise

except SGHTTPError as exc:

body = getattr(exc, 'body', b'')

try:

body = body.decode('utf-8', 'ignore') if isinstance(body, (bytes, bytearray)) else str(body)

except Exception:

pass

logging.error('SendGrid HTTPError: %s | payload=%s', body, payload)

raise

except (SGUnauthorizedError, SGForbiddenError, SGHTTPError) as exc:

body = getattr(exc, 'body', b'')

try:

body = body.decode('utf-8', 'ignore') if isinstance(body, (bytes, bytearray)) else str(body)

except Exception:

pass

if isinstance(exc, SGHTTPError):

logging.error('SendGrid auth error (%s): %s', exc.__class__.__name__, body)

else:

logging.error('SendGrid HTTPError: %s | payload=%s', body, payload)

raise

opaduchak · 2025-09-17T12:07:00Z

+        REGISTRATION_BULK_UPLOAD_FAILURE_DUPLICATES = 'registration_bulk_upload_failure_duplicates'
+
+        DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT = 'draft_registration_contributor_added_default'
+


Can't we add to simplify things?

Suggested change

def emit(

self,

user=None,

destination_address=None,

subscribed_object=None,

message_frequency='instantly',

event_context=None,

email_context=None,

is_digest=False,

save=True,

):

self.instance.emit(user, destination_address, subscribed_object, message_frequency, event_context, email_context, is_digest, save)

I am a bit confused by this, I think you mean a classmethod? or staticmethod instead?

no, this should be ordinary method, using the same logic as the instance property, replacing
NotificationType.Type.DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT.instance.emit
with
NotificationType.Type.DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT.emit

NotificationType.Type.DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT still an enum member, at that point so NotificationType.Type.DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT.emit wouldn't work. But I can see with you're other comments that the I can achieve that with those changes too.

it will work if we add emit method to the enum, the same way as instance property works

opaduchak · 2025-09-17T12:10:00Z

+
+class NotificationType(models.Model):
+
+    class Type(str, Enum):


Do we really need to make this nested enum? I'd rather make it full-fledged enum, which will result in drastivally simpler api:
from this:
NotificationType.Type.REVIEWS_SUBMISSION_STATUS.instance.emit()
to this (NotificationTypes is our full-fledged enum):
NotificationTypes.REVIEWS_SUBMISSION_STATUS.emit()

@cslzchen I think that's a much better idea, but involves a big diff, so I'm saving this for last in case there's more important things.

I agree but this will be a large change. Will this involve migration too? Will QA needs to retest it?

No migration just moving the Enum so it's not nested, it is more of a stylistic thing. if you look at how def instance is constructed, you can see how to improve like @opaduchak suggests, it would be just a textual change, but only to avoid the nested class and having a Type attribute. The Type enum is just the list of notification type names, they all have unique names, so it maps to the enum.

opaduchak · 2025-09-17T12:19:00Z

@@ -1,15 +1,20 @@
 from django.contrib import admin, messages


Should not this file be in admin package?

It's a bit confusing because we have the admin package which is custom HTML at admin.osf.io and the "admin admin" which is automatically generated UI. That's this file. This defines the "admin admin" which is found at admin.osf.io/admin, not admin.osf.io hence the name two admins.

Understood, excuse me, I mistook this for newly created file

opaduchak · 2025-09-17T12:33:30Z

-@run_postcommit(once_per_request=False, celery=True)
-@app.task(max_retries=5, default_retry_delay=60)
-def remove_supplemental_node_from_preprints(node_id):
-    AbstractNode = apps.get_model('osf.AbstractNode')
-
-    node = AbstractNode.load(node_id)
-    for preprint in node.preprints.all():
-        if preprint.node is not None:
-            preprint.node = None
-            preprint.save()


Why are we having this deleted? It doesn't seem to be related to notifications

Most of that file was no longer necessary, but that one function was still needed, so I moved it to notifications/listeners.py because as you will see it's basically a wrapper for a listener

…tions-data-model Conflicts will be fixed in next commit: * api/users/views.py * api_tests/users/views/test_user_settings.py

Johnetordoff force-pushed the add-new-notifications-data-model branch 4 times, most recently from 3a8b414 to 57b0bd1 Compare May 21, 2025 14:42

Johnetordoff force-pushed the add-new-notifications-data-model branch from 2b8ba0d to c449599 Compare June 9, 2025 13:40

Johnetordoff changed the base branch from feature/pbs-25-10 to refactor-notifications June 13, 2025 14:40

Johnetordoff force-pushed the add-new-notifications-data-model branch 3 times, most recently from ad18e9d to 300524c Compare July 3, 2025 14:20

Johnetordoff marked this pull request as ready for review July 8, 2025 20:05

Johnetordoff force-pushed the refactor-notifications branch from 2dee1b2 to 2dbcbf7 Compare July 10, 2025 15:50

Johnetordoff force-pushed the add-new-notifications-data-model branch 4 times, most recently from 9238258 to 37b419a Compare July 10, 2025 20:24

Johnetordoff force-pushed the refactor-notifications branch from afc3815 to b6bbeed Compare July 11, 2025 13:02

Johnetordoff force-pushed the add-new-notifications-data-model branch 14 times, most recently from 1a4314d to 2894a24 Compare July 18, 2025 14:38

Johnetordoff and others added 6 commits September 15, 2025 07:57

Merge pull request CenterForOpenScience#11299 from Ostap-Zherebetskyi…

412efc5

…/fix/reviews_signals [ENG-8851][ENG-8852] Preprint Moderator Reject: No emails sent when moderator rejects preprint submission

update TestArchiverUtils

940d3eb

added tests

251fa14

Merge pull request CenterForOpenScience#11298 from Ostap-Zherebetskyi…

e569514

…/fix/registration_fail_notification [ENG-8871] Registrations: No notifications sent when registration fails.

Merge pull request CenterForOpenScience#11300 from bodintsov/fix/no-n…

32c23f3

…otifications-send-when-account-merged [ENG-8869] Fix/make send email on account merge

fixed tests

75ddaa1

Johnetordoff commented Sep 15, 2025

View reviewed changes

Ostap-Zherebetskyi and others added 8 commits September 15, 2025 16:40

fix double notifications and AddonFileRenamed issues

06cdc0b

fix unit tests

6672f2d

Merge pull request CenterForOpenScience#11305 from Ostap-Zherebetskyi…

245016e

…/fix/file_event_notifications [ENG-8861] File Operations: Email content and subject not correct when file is renamed

Merge pull request CenterForOpenScience#11302 from bodintsov/fix/no-n…

7d7578a

…otification-send-when-primary-email-changed [ENG-8872] Fix/no notification send when primary email changed

fix submission notifications

a1b357f

fix unit tests

bc9dc6d

remove unused reviews submission notification function

98579bf

Merge pull request CenterForOpenScience#11308 from Ostap-Zherebetskyi…

c57313a

…/fix/submission_notifications [ENG-8848] Preprint pending submission notifications: Content of email is not relevant when new preprint is submitted to OSFPreprints

Johnetordoff commented Sep 16, 2025

View reviewed changes

cslzchen reviewed Sep 17, 2025

View reviewed changes

Ostap-Zherebetskyi added 2 commits September 17, 2025 12:56

fix Preprint file updated email

794aba4

fix file renamed email fix email digest fix digest formating

remove comment

db70e99

opaduchak reviewed Sep 17, 2025

View reviewed changes

Ostap-Zherebetskyi and others added 5 commits September 17, 2025 13:35

merge and fix unit tests

4afeeea

Merge remote-tracking branch 'upstream/develop' into add-new-notifica…

4bf1849

…tions-data-model Conflicts will be fixed in next commit: * api/users/views.py * api_tests/users/views/test_user_settings.py

Fix merge conflicts from develop (from PB&S release)

0582e6f

Merge migration

4e38386

Fix missing import due to merge

265c4d3

cslzchen approved these changes Sep 17, 2025

View reviewed changes

Fix mails import due to merge and fix tests

6f8cf89

brianjgeiger mentioned this pull request Sep 18, 2025

Revert "[ENG-8064] Add New Notifications Data Model (Refactor Notifications Phase 2)" #11318

Merged

		REGISTRATION_BULK_UPLOAD_FAILURE_DUPLICATES = 'registration_bulk_upload_failure_duplicates'

		DRAFT_REGISTRATION_CONTRIBUTOR_ADDED_DEFAULT = 'draft_registration_contributor_added_default'

+    def emit(
+                self,
+                user=None,
+                destination_address=None,
+                subscribed_object=None,
+                message_frequency='instantly',
+                event_context=None,
+                email_context=None,
+                is_digest=False,
+                save=True,
+        ):
+        self.instance.emit(user, destination_address, subscribed_object, message_frequency, event_context, email_context, is_digest, save)

Conversation

Johnetordoff commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opaduchak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opaduchak Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Johnetordoff commented May 20, 2025 •

edited

Loading

opaduchak Sep 17, 2025 •

edited

Loading